✨ adding filter& sort by analysis to Application-inventory table #2100
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HadasahR
requested review from
ibolton336,
sjd78 and
rszwajko
as code owners
HadasahR
changed the title
HadasahR
changed the title
HadasahR
changed the title
HadasahR
force-pushed
the
hadassah_application-inventoy-table-add-filtering-by-analysis
branch
from
0fa725a
to
c142659
Compare
HadasahR
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2100 +/- ##
==========================================
+ Coverage 39.20% 41.97% +2.77%
==========================================
Files 146 175 +29
Lines 4857 5629 +772
Branches 1164 1395 +231
==========================================
+ Hits 1904 2363 +459
- Misses 2939 3145 +206
- Partials 14 121 +107
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
rszwajko
requested changes
Sep 26, 2024
There was a problem hiding this comment.
The code for new functionality is added in the correct places but there are some problems with mapping state values. Unfortunately adding code to legacy class of this size is never easy :)
General comments:
- wording: the PR title is OK (you could start with a capital letter though), the description is repeating the title so you could skip it. However leave a link to the issue/bug you are fixing i.e. Resolves: [BUG] Need to be able to filter & sort by application analysis status #1745 . In git history you will find some examples
- check out your IDE configuration - it seems the IDE is not auto-formatting on save action. We have a script for linting - run from command line (in the project root)
npm run lint. You will see the list of all violations (we somehow accumulated few of them). Then you can fix (only) yours. General cleanup is a candidate for another PR (npm run lint -- --fixwill auto-fix all easy formatting problems in the whole project)
client/src/app/pages/applications/applications-table/applications-table.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/applications/applications-table/applications-table.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/applications/applications-table/applications-table.tsx
Outdated
Show resolved
Hide resolved
client/src/app/pages/applications/applications-table/applications-table.tsx
Show resolved
Hide resolved
|
Thanks, I'll check it out.
בתאריך יום ה׳, 26 בספט׳ 2024 ב-23:14 מאת Radoslaw Szwajkowski <
***@***.***>:
… ***@***.**** requested changes on this pull request.
The code for new functionality is added in the correct places but there
are some problems with mapping state values. Unfortunately adding code to
legacy class of this size is never easy :)
General comments:
1. wording: the PR title is OK (you could start with a capital letter
though), the description is repeating the title so you could skip it.
However leave a link to the issue/bug you are fixing i.e. Resolves:
#1745 <#1745> . In git
history you will find some examples
2. check out your IDE configuration - it seems the IDE is not
auto-formatting on save action. We have a script for linting - run from
command line (in the project root) npm run lint . You will see the
list of all violations (we somehow accumulated few of them). Then you can
fix (only) yours. General cleanup is a candidate for another PR (npm
run lint -- --fix will auto-fix all easy formatting problems in the
whole project)
------------------------------
In
client/src/app/pages/applications/applications-table/applications-table.tsx
<#2100 (comment)>:
> @@ -345,6 +348,7 @@ export const ApplicationsTable: React.FC = () => {
businessService: app.businessService?.name || "",
tags: app.tags?.length || 0,
effort: app.effort || 0,
+ analysis:app.tasks.currentAnalyzer?.state || ""
Sort values need to be the same as the values visible to the user.
Otherwise the sort order may feel "random".
------------------------------
In
client/src/app/pages/applications/applications-table/applications-table.tsx
<#2100 (comment)>:
> @@ -499,7 +503,24 @@ export const ApplicationsTable: React.FC = () => {
],
getItemValue: (item) => normalizeRisk(item.risk) ?? "",
},
+ {
+ categoryKey: "analysis",
+ title: t("terms.analysis"),
+ type: FilterType.multiselect,
+ placeholderText:
+ t("actions.filterBy", {
+ what: t("terms.analysis").toLowerCase(),
+ }) + "...",
+ selectOptions: Array.from(taskStateToAnalyze).map(
+ ([taskState, displayStatus]) => ({
Unfortunately few values are mapped to the same value (see the screenshot
below). You could remove duplicates in few ways but using a library is
probably the fastest - an example from our code base. Note also that the
values get sorted before the display.
Screenshot.from.2024-09-26.21-26-22.png (view on web)
<https://github.com/user-attachments/assets/d40772f2-8ff7-4684-8b33-d6cfaf63a9b5>
------------------------------
In
client/src/app/pages/applications/applications-table/applications-table.tsx
<#2100 (comment)>:
> @@ -499,7 +503,24 @@ export const ApplicationsTable: React.FC = () => {
],
getItemValue: (item) => normalizeRisk(item.risk) ?? "",
},
+ {
+ categoryKey: "analysis",
+ title: t("terms.analysis"),
+ type: FilterType.multiselect,
+ placeholderText:
+ t("actions.filterBy", {
+ what: t("terms.analysis").toLowerCase(),
+ }) + "...",
+ selectOptions: Array.from(taskStateToAnalyze).map(
+ ([taskState, displayStatus]) => ({
+ value: taskState,
+ label: t(`${displayStatus}`),
The displayed values should match the values that the user sees in the
table. Here the things get a bit tricky.
Basically you need to repeat the same flow as done in the table by ApplicationAnalysisStatus
component
<https://github.com/konveyor/tackle2-ui/blob/c04bb35390b3bd06e425dc8cead7cd92df2dd478/client/src/app/pages/applications/applications-table/applications-table.tsx#L929>.
There the received state is mapped via taskStateToAnalyze BUT later on
inside IconedStatus
<https://github.com/konveyor/tackle2-ui/blob/c04bb35390b3bd06e425dc8cead7cd92df2dd478/client/src/app/components/Icons/IconedStatus.tsx#L69>
it's mapped again - the label value is the value that the user sees.
------------------------------
In
client/src/app/pages/applications/applications-table/applications-table.tsx
<#2100 (comment)>:
> @@ -499,7 +503,24 @@ export const ApplicationsTable: React.FC = () => {
],
getItemValue: (item) => normalizeRisk(item.risk) ?? "",
},
+ {
+ categoryKey: "analysis",
+ title: t("terms.analysis"),
+ type: FilterType.multiselect,
+ placeholderText:
+ t("actions.filterBy", {
+ what: t("terms.analysis").toLowerCase(),
+ }) + "...",
+ selectOptions: Array.from(taskStateToAnalyze).map(
+ ([taskState, displayStatus]) => ({
+ value: taskState,
+ label: t(`${displayStatus}`),
+ })
+ ),
+ getItemValue: (item) => item?.tasks.currentAnalyzer?.state || "No Task",
The value here should match the value prop in the selectOptions list. If
this would not work please let me know - there are more options.
—
Reply to this email directly, view it on GitHub
<#2100 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BFOSM5AKDARWL6MXDB3TKVLZYRTK7AVCNFSM6AAAAABONHNEEKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZSGIYTOMJVGE>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
Signed-off-by: HadasahR <d025834828@gmail.com>
Signed-off-by: HadasahR <d025834828@gmail.com>
HadasahR
force-pushed
the
hadassah_application-inventoy-table-add-filtering-by-analysis
branch
from
185573c
to
614c152
Compare
Signed-off-by: HadasahR <d025834828@gmail.com>
HadasahR
force-pushed
the
hadassah_application-inventoy-table-add-filtering-by-analysis
branch
from
614c152
to
9d163db
Compare
rszwajko
requested changes
Oct 29, 2024
There was a problem hiding this comment.
The PR is going the right direction but it needs a preliminary refactoring - extracting presets creation to a re-usable function . I recommend to create a small PR for that and then re-use this functionality in this PR. Ping me if you need help with that.
| what: t("terms.analysis").toLowerCase(), | ||
| }) + "...", | ||
|
|
||
| selectOptions: Object.values(applications) |
There was a problem hiding this comment.
applications is already an array - no need to use Object.values
| }) + "...", | ||
|
|
||
| selectOptions: Object.values(applications) | ||
| .map((a) => { |
There was a problem hiding this comment.
You will need to re-use the same mapper for sorting so let's create a named function for that.
The steps remain the same i.e. (pseudo-code)
const mapAnalysisStateToLabel = (value: TaskState) => {
// for consistency let's use the same wa of converting as the rest of the code
const presetKey: IconedStatusPreset = getTaskStatus(value);
// as for now the presets are created inside the IconedStatus
// we need refactor this code and extract this functionality to a function i.e.buildPresets()
// this is best to do under a different PR
const presets = buildPresets(t);
// IMHO fallback to "Unknown" makes most sense
const label = presets[presetKey]?.label ?? t("terms.unknown");
return label;
}
Links:
getTaskStatus(value)
There was a problem hiding this comment.
the functions mentioned here are pushed in #2152
| return { value, label }; | ||
| }) | ||
| .filter((v, i, a) => a.findIndex((v2) => v2.label === v.label) === i) | ||
| .sort((a, b) => a.value.localeCompare(b.value)), |
There was a problem hiding this comment.
we should sort on label - note also that universalComparator helper adds some extra power to localeCompare ("natural sorting")
| @@ -345,6 +348,7 @@ export const ApplicationsTable: React.FC = () => { | |||
| businessService: app.businessService?.name || "", | |||
| tags: app.tags?.length || 0, | |||
| effort: app.effort || 0, | |||
| analysis: app.tasks.currentAnalyzer?.state || "", | |||
There was a problem hiding this comment.
we should sort on labels visible on the screen - see the propsed mapAnalysisStateToLabel below
HadasahR
added a commit
to HadasahR/tackle2-ui
that referenced
this pull request
Nov 3, 2024
Signed-off-by: HadasahR <d025834828@gmail.com>
rszwajko
pushed a commit
to rszwajko/tackle2-ui
that referenced
this pull request
Nov 6, 2024
Signed-off-by: HadasahR <d025834828@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
adding the ability to sort and filter by analysis status in the application inventory table.
before the change:
after the change :
UI test PR: konveyor/tackle-ui-tests#1238
this PR is using functions added in #2152